Support cross-locale slug lookup in language switcher #6634
Conversation
The language switcher previously failed when switching between locales where document slugs differ. For example, when switching from German "add-on-badges-abzeichen" back to English "add-on-badges", users would see a 404 error. This commit enhances the get_visible_document_or_404 function to: - Find correct documents across locales with different slugs - Look up documents by finding related translations in any locale - Maintain backward compatibility with existing behavior Added comprehensive tests to verify the new functionality works with: - Direct locale/slug matches - Cross-locale lookups (any locale to any locale) - Document fallbacks when translations don't exist
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the language switcher functionality by adding support for cross-locale slug lookups to prevent 404 errors when document slugs differ between locales. Key changes include:
- Updating the get_visible_document_or_404 logic to search for translations in both default and non-default locales.
- Implementing separate handling for default and non-default locale lookups.
- Adding comprehensive tests to validate direct locale matches, cross-locale lookups, and fallback behaviors.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kitsune/wiki/utils.py | Updated lookup logic to support cross-locale searches and translation fallback. |
| kitsune/wiki/tests/test_utils.py | Added new tests covering direct matches, cross-locale lookups, fallback scenarios, and access restrictions. |
|
|
||
| # When switching from French to English, we should find the English document | ||
| # even though we're looking for the French slug in English | ||
| doc = get_visible_document_or_404( |
There was a problem hiding this comment.
Isn't this the same assertion with the German document?
There was a problem hiding this comment.
I'm not sure I follow you - all these tests have subtle differences, either slug or locale or other flags. Let me know if I've missed something.
|
I dropped most of the comments from the test file. I left many comments in the |
kitsune/wiki/utils.py
Outdated
| # see if we can find a visible translation via its parent. | ||
| slug = kwargs.get("slug") | ||
|
|
||
| if slug and look_for_translation_via_parent: |
There was a problem hiding this comment.
Again many redundant comments
| if translation: | ||
| return translation | ||
|
|
||
| if not look_for_translation_via_parent or locale == settings.WIKI_DEFAULT_LANGUAGE: |
There was a problem hiding this comment.
Is there a reason this check is so late in the code after all the expensive operations above? This should be combined with the earlier 404.
There was a problem hiding this comment.
Yes - the second check after the loop prevents the final fallback when we're in the default language. Checking it earlier breaks the case where a user requests a document in one locale but is using a slug from another. So - check to see if we should bother finding a translation.
kitsune/wiki/utils.py
Outdated
| if slug and look_for_translation_via_parent: | ||
| # Find documents with this slug in other locales | ||
| other_docs_qs = ( | ||
| Document.objects.filter(slug=slug).exclude(locale=locale).select_related("parent") |
There was a problem hiding this comment.
Shouldn't we use visible here too like above instead of filter?
There was a problem hiding this comment.
My thoughts were that since we are looking for a path to a document through other documents, the documents we look at to move to a valid document should all be collected. We check for visibility later before presenting a final document. What if a document is restricted in one language but not in another?
| Document.objects.filter(slug=slug).exclude(locale=locale).select_related("parent") | ||
| ) | ||
|
|
||
| if locale == settings.WIKI_DEFAULT_LANGUAGE: |
There was a problem hiding this comment.
This feels inefficient. We have basically 2 blocks of code (if/else). Each block has the same loop and a call to translated_to. This should have been the other way. Loop once over the docs and check what you need for each one
The language switcher previously failed when switching between locales where document slugs differ. For example, when switching from German "add-on-badges-abzeichen" back to English "add-on-badges", users would see a 404 error.
This commit enhances the get_visible_document_or_404 function to:
Added comprehensive tests to verify the new functionality works with: